Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix highlight config so it doesn't require a refresh to apply #10346

Merged
merged 2 commits into from
Feb 14, 2017
Merged

Fix highlight config so it doesn't require a refresh to apply #10346

merged 2 commits into from
Feb 14, 2017

Conversation

lukasolson
Copy link
Member

@lukasolson lukasolson commented Feb 14, 2017 •
edited
Loading

Prior to this PR, when you would go into the advanced settings and change the doc_table:highlight to false (or true), it would require refreshing the page before it would be applied. This was confusing.

This PR fixes this behavior such that the config is checked each time we go to apply highlighting instead of only when the page loads, and adds a test surrounding this behavior.

Fixes #7796.

@Bargs
Copy link
Contributor

Bargs commented Feb 14, 2017 •
edited
Loading

This looks pretty good. I did find one edge case. If you add a saved search from a pre 5.3 installation to a dashboard you'll still get highlighting even if doc_table:highlight is false because the highlighting config is stored with the saved object. It works on discover because we always set highlightAll to true which causes the saved object highlighting to get ignored. I think if we do the same on Dashboard we should be good.

@lukasolson
Copy link
Member Author

Good catch, @Bargs.

@lukasolson
Copy link
Member Author

@Bargs Could you take another look? Thanks!

Bargs
Bargs approved these changes Feb 14, 2017
View reviewed changes
Copy link
Contributor

@Bargs Bargs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@lukasolson
Copy link
Member Author

jenkins, test this

@Bargs Bargs mentioned this pull request Feb 14, 2017
Closed
@lukasolson lukasolson merged commit bd966db into elastic:master Feb 14, 2017
lukasolson added a commit that referenced this pull request Feb 15, 2017
@lukasolson
Fix highlight config so it doesn't require a refresh to apply (#10346)
d54757b 
* Acknowledge doc_table:highlight config even after init

* Use highlightAll on dashboard
@lukasolson
Copy link
Member Author

lukasolson commented Feb 15, 2017 •
edited
Loading

Backported to 5.3 in 3b34f16.
Backported to 5.x (5.4) in d54757b.

@lukasolson lukasolson mentioned this pull request Feb 17, 2017
Merged
@bhavyarm
Copy link
Contributor

bhavyarm commented Mar 3, 2017

@lukasolson I had discover and advanced settings open in two separate tabs(chrome latest). Then I tried setting the value of doc_table:highlight and looked at the results in discover. The settings didn't get applied till I refreshed the page. Is this expected.

cc @Bargs this works fine otherwise. What we saw together in zoom - I couldn't reproduce it in local. Thanks!

lukasolson added a commit that referenced this pull request Apr 10, 2017
@lukasolson
Fix highlight config so it doesn't require a refresh to apply (#10346)
b800f44 
* Acknowledge doc_table:highlight config even after init

* Use highlightAll on dashboard
@lukasolson lukasolson deleted the fix-highlight-config branch March 27, 2018 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weltenwort weltenwort weltenwort approved these changes

@Bargs Bargs Bargs approved these changes

Assignees

@weltenwort weltenwort

@Bargs Bargs

Labels
Feature:Discover Discover Application v5.3.0 v5.4.0 v6.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

doc_table:highlight = false does not appear to be working
4 participants
@lukasolson @Bargs @bhavyarm @weltenwort